docs(ui): add ButtonGroup stories to storybook#1964
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Storybook configuration and button stories: adds the storybook-i18n addon and LOCALE_CHANGED handling, creates a dedicated theme module (npmxDark) used by manager and preview, injects docs theme and a docs canvas background CSS, and removes a fault-tolerant wrapper around vue-docgen-plugin. Moves Button stories from a deleted Base.stories.ts into new ButtonBase.stories.ts and ButtonGroup.stories.ts, and adds component name declarations via defineOptions in Button Base and Group components. Updates package.json and pnpm-workspace.yaml to include storybook-i18n and extends knip.ts ignoreDependencies. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.storybook/preview.ts (1)
85-85: Avoid usinganytype for i18n instance.The
i18nInstanceis typed asany, which bypasses TypeScript's type checking. Consider using the appropriate type fromvue-i18nor at minimum a more descriptive interface.🛠️ Suggested improvement
- let i18nInstance: any = null + let i18nInstance: { setLocale: (locale: string) => void } | null = null
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b33c7bfb-2b5b-4119-9765-c992b32e7ac6
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.storybook/main.ts.storybook/manager.ts.storybook/preview-head.html.storybook/preview.ts.storybook/theme.tsapp/components/Button/Base.stories.tsapp/components/Button/Base.vueapp/components/Button/Button.stories.tsapp/components/Button/ButtonGroup.stories.tsapp/components/Button/Group.vuepackage.jsonpnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- app/components/Button/Base.stories.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3e058ab-44b3-47cc-ab0d-2d13215a73a5
📒 Files selected for processing (1)
app/components/Button/ButtonBase.stories.ts
| export const Default: Story = { | ||
| args: { | ||
| default: 'Button Text', | ||
| }, | ||
| } |
There was a problem hiding this comment.
Default story may not render slot content without a render function.
All other stories use explicit render functions with templates to handle slot content, but Default relies on args.default without a render function. In Vue Storybook, this pattern typically requires either a render function or specific argTypes configuration to pass slot content properly.
Consider adding a render function for consistency and to ensure the button text displays:
Proposed fix
export const Default: Story = {
- args: {
- default: 'Button Text',
- },
+ render: args => ({
+ components: { ButtonBase },
+ setup() {
+ return { args }
+ },
+ template: `<ButtonBase v-bind="args">Button Text</ButtonBase>`,
+ }),
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const Default: Story = { | |
| args: { | |
| default: 'Button Text', | |
| }, | |
| } | |
| export const Default: Story = { | |
| render: args => ({ | |
| components: { ButtonBase }, | |
| setup() { | |
| return { args } | |
| }, | |
| template: `<ButtonBase v-bind="args">Button Text</ButtonBase>`, | |
| }), | |
| } |
There was a problem hiding this comment.
Keeping the default story with the default render allow to someone viewing the storybook to change the story to alternative text in place of Button Text. The other stories use the template render in order for the internationalization to work.
There was a problem hiding this comment.
@cylewaitforit, that makes perfect sense! Using args.default without a render function is the correct approach here — it exposes the slot content as a Storybook control, allowing users to interactively edit the button text directly from the controls panel. The render function approach in the other stories is needed for i18n to work, but would prevent that interactive editing for the Default story. Thanks for the clarification!
ʕ •`ᴥ´• ʔ
✏️ Learnings added
Learnt from: cylewaitforit
Repo: npmx-dev/npmx.dev PR: 1964
File: app/components/Button/ButtonBase.stories.ts:21-25
Timestamp: 2026-03-06T18:16:31.538Z
Learning: In the npmx.dev project's ButtonBase.stories.ts (app/components/Button/ButtonBase.stories.ts), the `Default` story intentionally uses `args.default` without a render function so that Storybook users can interactively edit the slot content (button text) via the controls panel. Other stories (e.g. Primary, Secondary, etc.) use explicit render functions with templates specifically to support storybook-i18n internationalization. Do not flag the `Default` story's lack of a render function as an issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1335
File: app/components/Compare/FacetSelector.vue:72-78
Timestamp: 2026-02-10T15:47:40.710Z
Learning: In the npmx.dev project, the `ButtonBase` component (app/components/ButtonBase.vue or similar) provides default `border border-border` styles. When styling ButtonBase instances, avoid adding duplicate `border` classes as this triggers the HTML validator error `no-dup-class` and fails CI.
Learnt from: cylewaitforit
Repo: npmx-dev/npmx.dev PR: 1964
File: .storybook/preview-head.html:1-6
Timestamp: 2026-03-06T17:44:59.085Z
Learning: In the npmx.dev project, the `.storybook/preview-head.html` file sets `background-color` on `.docs-story` via the `--bg` CSS custom property to fix the Autodocs canvas background. This is an intentional workaround for a known Storybook bug (storybookjs/storybook#30928) where `addon-themes` and `parameters.docs.theme` do not apply to the individual story canvas iframes in the Autodocs tab. Removing this file causes the autodocs pages to not display correctly.
|
Could you mark this as ready for review when you are? Then I'll take a look! |
cylewaitforit
force-pushed
the
sb-button-group
branch
from
57becd4 to
a923dd3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Button/ButtonGroup.stories.ts (1)
26-106: Reduce the duplicated story markup.Each story now maintains the same example twice: once in
docs.source.codeand again inrender().template. That is easy to drift on the next edit and makes these stories more expensive to maintain than they need to be. A small helper or shared story data would keep the docs snippet and live render in sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a4a594fa-72da-4dbd-b9df-2f2dfca19696
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.storybook/main.ts.storybook/manager.ts.storybook/preview-head.html.storybook/preview.ts.storybook/theme.tsapp/components/Button/Base.stories.tsapp/components/Button/Base.vueapp/components/Button/ButtonBase.stories.tsapp/components/Button/ButtonGroup.stories.tsapp/components/Button/Group.vueknip.tspackage.jsonpnpm-workspace.yaml
💤 Files with no reviewable changes (1)
- app/components/Button/Base.stories.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- .storybook/theme.ts
- app/components/Button/Group.vue
- knip.ts
- pnpm-workspace.yaml
- app/components/Button/ButtonBase.stories.ts
- .storybook/main.ts
- app/components/Button/Base.vue
- package.json
@ghostdevv Ready when you have time. |
cylewaitforit
force-pushed
the
sb-button-group
branch
from
51ec2db to
9f849a1
Compare
3 participants
🔗 Linked issue
Part of #1841
🧭 Context
Adds stories for ButtonGroup
Preview npmx Storybook on Chromatic
📚 Description